-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/telegram bot #455
base: develop
Are you sure you want to change the base?
Feature/telegram bot #455
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sentax - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
TELEGRAM_CHANNEL_ID = os.environ.get("TELEGRAM_CHANNEL_ID") | ||
TELEGRAM_API_TOKEN = os.environ.get("TELEGRAM_API_TOKEN") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Environment variables should have default values.
Consider providing default values for TELEGRAM_CHANNEL_ID
and TELEGRAM_API_TOKEN
to avoid potential NoneType
errors if the environment variables are not set.
TELEGRAM_CHANNEL_ID = os.environ.get("TELEGRAM_CHANNEL_ID") | |
TELEGRAM_API_TOKEN = os.environ.get("TELEGRAM_API_TOKEN") | |
TELEGRAM_CHANNEL_ID = os.environ.get("TELEGRAM_CHANNEL_ID", "") | |
TELEGRAM_API_TOKEN = os.environ.get("TELEGRAM_API_TOKEN", "") |
} | ||
try: | ||
response = requests.post(url, data=payload) | ||
return {"ok":True} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider logging the exception.
In the except
block, consider logging the exception to help with debugging in case of failures.
return {"ok":True} | |
return {"ok": True} | |
except Exception as e: | |
logging.error(f"Exception occurred: {e}") | |
return {"ok": False} |
core/telegram.py
Outdated
log_hash = hash(message) | ||
current_time = time.time() | ||
|
||
# Check if the log has been sent before or if it has been more than 1 hour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (typo): Comment does not match the code.
The comment mentions '1 hour', but the code uses MIN_INTERVAL
which is set to 10 seconds. Update the comment to accurately reflect the code logic.
current_time = time.time() | ||
|
||
# Check if the log has been sent before or if it has been more than 1 hour | ||
if current_time - log_cache[log_hash] > MIN_INTERVAL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Potential race condition with log_cache.
Accessing and modifying log_cache
without any locking mechanism could lead to race conditions in a multi-threaded environment. Consider using a thread-safe data structure or adding appropriate locking.
res = self.middleware.log_message("Test log message") | ||
self.assertEqual(res['ok'], True) | ||
res = self.middleware.log_message("Test log message") | ||
self.assertEqual(res['ok'], False) | ||
# delay | ||
time.sleep(TELEGRAM_MIN_LOG_INTERVAL) | ||
res = self.middleware.log_message("Test log message") | ||
self.assertEqual(res['ok'], True) | ||
res = self.middleware.log_message("Test log message") | ||
self.assertEqual(res['ok'], False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method
)
ad3bdb7
to
7f69504
Compare
Implemented a feature for monitoring porpuses that uses telegram bot API and django middleware.
2 new env variables added:
TELEGRAM_API_TOKEN : father bot given token
TELEGRAM_CHANNEL_ID : channel id in 2 formats (number[-1001609474777], string["@ChannelID"])
bot should be added to the channel and allowed to send messages in that channel before.